-
Notifications
You must be signed in to change notification settings - Fork 320
unreads: Add/use locatorMap, to efficiently locate messages #1703
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisbobbe! LGTM, two small comments below.
lib/model/unreads.dart
Outdated
DmMessage() => DmNarrow.ofMessage(message, selfUserId: selfUserId), | ||
}; | ||
locatorMap[event.message.id] = narrow; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe merge both switches? To avoid a copy of DmNarrow
and more specifically of allRecipientIds
list in DmNarrow.ofMessage
.
switch (message) {
case StreamMessage():
final narrow = TopicNarrow.ofMessage(message);
locatorMap[event.message.id] = narrow;
_addLastInStreamTopic(message.id, message.streamId, message.topic);
case DmMessage():
final narrow = DmNarrow.ofMessage(message, selfUserId: selfUserId);
locatorMap[event.message.id] = narrow;
_addLastInDm(message.id, narrow);
}
lib/model/unreads.dart
Outdated
if (messageIds.isEmpty) { | ||
newlyEmptyTopics.add(topic); | ||
} | ||
/// Remove any of [idsToRemove] that are in [streams]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
/// Remove any of [idsToRemove] that are in [streams]. | |
/// Remove any of [idsToRemove] that are in [streams]. |
f3988b0
to
02f9779
Compare
Thanks for the review! Revision pushed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @chrisbobbe for building this, and @rajveermalviya for the previous review! Comments below.
lib/model/unreads.dart
Outdated
/// All unread messages, as: message ID → narrow ([TopicNarrow] or [DmNarrow]). | ||
/// | ||
/// Enables efficient [isUnread] and efficient lookups in [streams] and [dms]. | ||
final Map<int, Narrow> locatorMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be a bit more specific, I think:
final Map<int, Narrow> locatorMap; | |
final Map<int, SendableNarrow> locatorMap; |
Not sure the optimizer will get any useful information from that. But it seems good for matching our intent — the doc says basically the same thing a few lines above.
lib/model/unreads.dart
Outdated
/// All unread messages, as: message ID → narrow ([TopicNarrow] or [DmNarrow]). | ||
/// | ||
/// Enables efficient [isUnread] and efficient lookups in [streams] and [dms]. | ||
final Map<int, Narrow> locatorMap; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In principle this is a private detail of this class. It might be convenient to access it in tests; but let's make it @visibleForTesting
, if not private.
lib/model/unreads.dart
Outdated
final messageIds = streams[narrow.streamId]?[narrow.topic]; | ||
if (messageIds == null) continue; | ||
|
||
messageIds.remove(messageId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's common that a mark-read event has all its messages from the same conversation, or many messages in each of a small number of conversations. When that happens, this version will do some repeated work on those:
- looking up the list in the two maps under
streams
; - more significantly, scanning through the list for the conversation to find the message to remove, and then going through the remainder of the list to move the other entries up to keep the list contiguous.
The second part in particular makes it take quadratic time to mark all the messages in a long conversation as read.
To avoid that, let's have this method first go through the list of message IDs and look them up in locatorMap, and assemble a to-do list grouped by conversation. Then it can go through that to-do list and act on each conversation just once.
For an example, see the implementation of mark-as-unread above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(similarly for _removeAllInDms
)
test/model/unreads_test.dart
Outdated
check(model.locatorMap).not((it) => it.containsKey(message.id)); | ||
continue; | ||
} | ||
switch (message) { | ||
case StreamMessage(): | ||
check(model.locatorMap)[message.id].equals(TopicNarrow.ofMessage(message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These checks don't fully cover the invariant that I'd want locatorMap to maintain: it should contain exactly the messages that are in streams
and dms
, and agree with those data structures about where each message is. These checks appear to not cover the possibility that some messages linger in locatorMap that aren't in this messages
argument.
I think the cleanest way to check that would be as a separate piece of logic at the end of this function. Let's build up the whole expected locatorMap, based on the other data structures; then deepEquals
to compare.
Fixes zulip#332. Really the same optimization as described for zulip-mobile in zulip/zulip-mobile#4684 , and makes mark-as-read take 0ms (to the nearest ms) down from 3 or 4, in my testing, and so fixes zulip#332.
02f9779
to
8be5e12
Compare
Thanks!! Revision pushed. |
Fixes #332.
Really the same optimization as described for zulip-mobile in
zulip/zulip-mobile#4684
, and makes mark-as-read take 0ms (to the nearest ms) down from 3 or 4, in my testing, and so fixes #332.